Open Bug 1485588 Opened 7 years ago Updated 3 years ago

[clang-tidy] Prohibit the use of the C standard library for character classification

Categories

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Version:
63 Branch
Type:
enhancement

Tracking

(Not tracked)

Status:
REOPENED

People

(Reporter: hsivonen, Assigned: freddy)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1485588 - Part 1: Clang-plugin checker for locale-specific functions. r?andi.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1485588 - Part 2: Suppress warnings in a gtest file. r?andi
47 bytes, text/x-phabricator-request
Details | Review
The C standard library has functions for classifying characters, such as isalpha() and isdigit(). Using these is always wrong. They are wrong to use when working with natural-language text, because they only deal with values in the range of char or the EOF constant instead of dealing with the full Unicode range. They are wrong when working with computer syntaxes, because they are locale-dependent. Even the ones that aren't locale-dependent according to the C standard are locale-dependent on Windows. https://stackoverflow.com/questions/2898228/can-isdigit-legitimately-be-locale-dependent-in-c I suggest adding an analysis that complains about the use of these functions (or their C++ analogs under the std:: namespace) and suggests the use of mbft/TextUtils.h instead.
This is very easy to implement, it can be done very easy at the matcher level, an example can be found here: https://bugzilla.mozilla.org/show_bug.cgi?id=1391711.
Also strcasecmp and strncasecmp from strings.h are locale-dependent.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1452693#c1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
not fixed, sorry
Severity: normal → enhancement
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: nobody → jorendorff

Work in progress. I don't know how to add exceptions for existing code. Is
there an existing lint that has something like that?

There are several copies of gtest in our tree, all listed in tools/rewriting/ThirdPartyPaths.txt. Nevertheless, they are triggering these warnings as though they were first-party code, because we link them into the OBJDIR:

$OBJDIR/dist/include/gtest/internal/gtest-port.h -> $SRCDIR/testing/gtest/gtest/include/gtest/internal/gtest-port.h

Is this a known issue?

Flags: needinfo?(bpostelnicu)

Yes, please see this workaround https://searchfox.org/mozilla-central/source/build/clang-plugin/CustomMatchers.h#137

Flags: needinfo?(bpostelnicu)
See Also: → 1650637
Attachment #9160527 - Attachment description: Bug 1485588 - Clang-plugin checker for locale-specific functions. r?hsivonen. → Bug 1485588 - Part 1: Clang-plugin checker for locale-specific functions. r?andi.
Pushed by jorendorff@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fad2fb1e60ee Part 1: Clang-plugin checker for locale-specific functions. r=hsivonen. https://hg.mozilla.org/integration/autoland/rev/3f6600510057 Part 2: Suppress warnings in a gtest file. r=andi

Hmm. strcasecmp doesn't exist on Windows. By far the easiest thing is to delete that particular part of the test, so I guess I'll do that.

(In reply to Jason Orendorff [:jorendorff] from comment #13)

Hmm. strcasecmp doesn't exist on Windows. By far the easiest thing is to delete that particular part of the test, so I guess I'll do that.

Does that mean we should be checking for the Windows equivalents as well?

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/stricmp-wcsicmp-mbsicmp-stricmp-l-wcsicmp-l-mbsicmp-l?view=vs-2019

(In reply to Nathan Froyd [:froydnj] from comment #14)

Does that mean we should be checking for the Windows equivalents as well?

It indeed appears that we should block _stricmp, _wcsicmp, and _mbsicmp, too.

Flags: needinfo?(jorendorff)
Priority: -- → P3
Assignee: jorendorff → fbraun
Product: Firefox Build System → Developer Infrastructure
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: